Skip to content

Support Progress Flow for McpClient #389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

134130
Copy link

@134130 134130 commented Jul 12, 2025

Motivation and Context

How Has This Been Tested?

Breaking Changes

  • No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@134130 134130 marked this pull request as draft July 12, 2025 01:38
@tzolov
Copy link
Contributor

tzolov commented Jul 15, 2025

@134130 looks like this work overlaps with the #300?

Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@134130 ,
I left few comments. Also please do not try to format the McpSchema. Apply only your changes.
Also it looks like it would be easier to start over in a new PR on top of the latest origin/main and apply copy on only the meaningful changes from (to replace #389 and #300)
What do you think?

* @param progressNotification The progress notification to send
* @return A Mono that completes when the notification has been sent
*/
public Mono<Void> progressNotification(McpSchema.ProgressNotification progressNotification) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be mirrored in the McpSyncServerExchange

@@ -23,6 +23,7 @@
import io.modelcontextprotocol.util.Assert;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import reactor.util.annotation.Nullable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the reactor.util.annotation.Nullable;

@134130
Copy link
Author

134130 commented Jul 15, 2025

@tzolov Thank you for your comment!

Yes, the PR depends on #300 and will be rebased after the #300 is merged.

I've tried to separate the PR for Server and Client implementation, as the #298 task list.

How do you think about this? Do you think separating the PR is meaningful? And do you want a new PR with closing #300 and #389?

About McpSchema formatting

Of course! I see the #392, and I can rework to reduce the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants